O3-1242 App crashes on logout #423
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
brandones
commented
May 6, 2022
Comment on lines
+166
to
+168
| devMiddleware: { | ||
| writeToDisk: true, | ||
| }, |
There was a problem hiding this comment.
Unrelated: this makes it so that yarn run:shell plays nicer with yarn serveing the apps.
Noted with thanks! 😊 |
ZacButko
approved these changes
May 6, 2022
packages/apps/esm-login-app/src/redirect-logout/redirect-logout.component.tsx
Outdated
Show resolved
Hide resolved
| <BrowserRouter basename={window.spaBase}> | ||
| <Route | ||
| exact | ||
| path="/login(/confirm)?" |
There was a problem hiding this comment.
Just checking, I haven't seen this syntax from react-router-dom before... doesn't look like plain text and doesn't look like regex?
There was a problem hiding this comment.
I didn't write this, but it looks like regex to me. Parentheses for grouping and a question mark for "optional;" i.e. this will match /login or /login/confirm.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Requirements
For changes to apps
If applicable
Summary
This fixes the broken logout functionality. It also fixes a problem where clicking "Change location" from the user menu in the primary navigation bar would lead to an "An error has occurred. Please try reloading the page." page.
The general idea of the fix is to make it so that
useSessionuses Suspense, so that it always either throws or returns a valid session object—it can no longer returnnull.There is a substantial bit of refactoring worked in. I changed
getCurrentUserto use Unistore rather than RxJS for state management, mostly because I have a much better understanding of how Unistore works, and am confident that it will behave synchronously when I expect it to (the behavior of Observables is ambiguous in this regard).I did my best to document the hook's implementation, since its complexity is increased substantially.
Screenshots
Related Issue
https://issues.openmrs.org/browse/O3-1242
Other
Ideally, this should be tested by E2E tests. CC @jayasanka-sack .